New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add golangci-lint #569
Conversation
a35232c
to
1b9bab2
Compare
Transitioning to a draft while I fix tests |
Sure! If possible it would be awesome if the |
52d6a8d
to
d57a76f
Compare
@baez90 I've removed the dependency via refactor: remove gotest.tools/v3 usage |
7b09803
to
e7e7d7f
Compare
dd09bfe
to
7e25a6b
Compare
Codecov Report
@@ Coverage Diff @@
## main #569 +/- ##
==========================================
+ Coverage 13.54% 15.48% +1.93%
==========================================
Files 15 13 -2
Lines 2001 1770 -231
==========================================
+ Hits 271 274 +3
+ Misses 1684 1448 -236
- Partials 46 48 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
7e25a6b
to
304f44c
Compare
b8c0ad8
to
0ec36f4
Compare
@@ -29,7 +30,12 @@ func ExampleExecStrategy() { | |||
panic(err) | |||
} | |||
|
|||
defer localstack.Terminate(ctx) // nolint: errcheck | |||
defer func() { | |||
if err := localstack.Terminate(ctx); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdyt about using testcontainers.Cleanup(t, ctx, container)
, not only here but everywhere, for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testcontainers.Cleanup
shouldn't have been here. I agree it's a nice helper function, but it'll be moved to a separate PR.
0ec36f4
to
bea343a
Compare
bea343a
to
f19472d
Compare
if _, err := sock.WriteString(strings.Join(labelFilters, "&")); err != nil { | ||
continue | ||
} | ||
|
||
if _, err := sock.WriteString("\n"); err != nil { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdelapenya Can you confirm this is okay? Tests pass, but I'd like to be extra careful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given in https://github.com/testcontainers/testcontainers-go/pull/569/files#diff-e8ab4ec17e978db949cd15726ba0efa9c89fd714e7e6e0aa537ed7fc1bbde7fdR138 we continue on a flush failure, I'd say that it's okay
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this! It will help decouple the main pipeline from the static analysis, where we can go adding more linters if/when needed.
At the same time, golangci-lint is becoming a de-facto standard on linting Go projects, so great addition!
Finally thanks for your support during the review, there were a lot of comments, but I think this PR is great now. Thank you! 👏
* main: feat: add golangci-lint (#569)
Context
This pull request introduces golangci-lint into the existing CI/CD Pipeline to increase the project's code health. Applying
go fmt
to all files, removing blank identifier usage in all Wait Strategies for the implicit acceptance of the interface, removes deprecated functions/methods, and fixes a lot of missing error handling.Additionally, in 9db062a I've exposed thenoopStrategyTarget
aswaittest.NopStrategyTarget
for others to consume in their own tests. It was renamed to match theNop
naming style used in Go's standard lib. This does not need to be apart of these changes and can be removed from this PR if required.